Remove the dead use_adjoint_scaling parameter and adjoint_scaling attribute - #478
Merged
Merged
Conversation
…ribute `use_adjoint_scaling` has no effect on either remaining transformer. Both `TransformerDFT.image_from` and `TransformerNUFFT.image_from` accepted it and never referenced it in any code path, so passing `True` silently did nothing. It was not always dead, and it is worth recording where the work went: - `TransformerDFT` never applied it. - `TransformerNUFFTPyNUFFT` did — `image *= self.adjoint_scaling` was a Kaiser-Bessel compensation that only the pynufft backend needed. That class was removed in #475. - `TransformerNUFFT` applied it until bd18a76 (2026-05-22), which deleted the multiplication on the grounds that "the nufftax adjoint is already the mathematical adjoint and needs no extra scaling", making it scale-consistent with `TransformerDFT` in `apply_sparse_operator`. With pynufft gone there is no backend that needs the factor, so both the parameter and the `adjoint_scaling` attribute (`4 * N_y * N_x`) are removed rather than left as an inert part of the public surface. A caller still passing the keyword now gets a `TypeError` instead of a silent no-op, which is the safer failure: the value was load-bearing for one deleted class, so silence is exactly the wrong response. `Interferometer.apply_sparse_operator` was the only caller and no longer passes it. Scale-consistency across the two transformers is unchanged — it comes from both returning the plain mathematical adjoint, not from this flag. Verified: `TransformerNUFFT.image_from` and `TransformerDFT.image_from` agree to 1.562e-13 relative both before and after this change (identical figure), and `True` vs `False` was bit-identical beforehand. Had the factor still applied it would have scaled the dirty image by 4096x on a 32x32 grid. Suites green: autoarray 1179 passed (including test__curvature_matrix__interferometer_sparse_operator__delaunay__dft_and_nufft_match, the parity test bd18a76 added for this path), autogalaxy 1103 passed / 1 skipped, autolens 532 passed / 1 skipped.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
use_adjoint_scalinghas no effect on either remaining transformer. BothTransformerDFT.image_fromandTransformerNUFFT.image_fromaccepted it and never referenced it in any code path — including all four ofTransformerNUFFT's (jax one-shot, jax chunked, numpy one-shot, numpy chunked). PassingTruesilently did nothing.Follow-up to #475, which flagged this but deliberately left it alone rather than widening a dependency removal.
It was not always dead — where the work actually went
adjoint_scaling?TransformerDFTTransformerNUFFTPyNUFFTimage *= self.adjoint_scaling, the only real user. Removed in #475.TransformerNUFFTbd18a769(2026-05-22), then deliberately removedbd18a769states the reason: the4·N_y·N_xfactor "is a Kaiser-Bessel compensation that only the legacy pynufft variant needs; the nufftax adjoint is already the mathematical adjoint and needs no extra scaling." With pynufft gone there is no backend that needs it.Why remove the attribute too, not just the parameter
adjoint_scalingwas public and inert. Leaving it invites someone to apply it by hand and silently scale a dirty image by 4096× on a 32×32 grid. A caller still passing the keyword now gets aTypeErrorrather than a silent no-op — the safer failure, since the value was genuinely load-bearing for one now-deleted class.Scale-consistency across the two transformers is unaffected: it comes from both returning the plain mathematical adjoint, not from this flag.
API Changes
Breaking: the
use_adjoint_scalingkeyword is removed fromTransformerDFT.image_fromandTransformerNUFFT.image_from, and theadjoint_scalingattribute is removed from both classes. No migration is needed for correctness — the keyword had no effect on either class, so removing the call site changes no numbers. Anyone readingtransformer.adjoint_scalingand applying it manually was introducing a 4096×-class error and should stop.See full details below.
Test Plan
TransformerNUFFT.image_fromvsTransformerDFT.image_from: 1.562e-13 relative, identical before and after this changeuse_adjoint_scaling=TruevsFalsewas bit-identical (max|diff| = 0.000e+00) on both classes before removalTypeError(loud, not silent)pytest test_autoarray— 1179 passed, includingtest__curvature_matrix__interferometer_sparse_operator__delaunay__dft_and_nufft_match, the parity testbd18a769added for exactly this pathFull API Changes (for automation & release notes)
Removed
TransformerDFT.image_from(..., use_adjoint_scaling=)— keyword removed; it was never referencedTransformerNUFFT.image_from(..., use_adjoint_scaling=)— keyword removed; unreferenced sincebd18a769TransformerDFT.adjoint_scaling— attribute removedTransformerNUFFT.adjoint_scaling— attribute removedChanged Signature
TransformerDFT.image_from(visibilities, xp=np)— was(visibilities, use_adjoint_scaling=False, xp=np)TransformerNUFFT.image_from(visibilities, xp=np)— was(visibilities, use_adjoint_scaling=False, xp=np)Changed Behaviour
Migration
transformer.image_from(visibilities=vis, use_adjoint_scaling=True)transformer.image_from(visibilities=vis)Interferometer.apply_sparse_operatorwas the only in-repo caller and is updated.Note for anyone with pre-2026-05-22 results: if you used
TransformerNUFFTwithuse_adjoint_scaling=Truebeforebd18a769, the factor was applied then and is not applied now. Results generated before that commit differ from anything regenerated today by4 * N_y * N_x. That change landed in May and is not introduced here.Generated by Claude Code